fix(upgrade): need to call markForCheck on child changeDetectorRef #22209

smdunn · 2018-02-13T22:22:08Z

PR Checklist

Please check if your PR fulfills the following requirements:

[x ] The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
[n/a ] Tests for the changes have been added (for bug fixes / features)
[n/a ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

I found a bug within matCheckbox. I added a disabled class to the checkbox based on a method call. When disabled, the checkbox had class disabled and therefore had grey css etc, but if it was still clicked on, it had a click event which added a disabled class to the input within the checkbox. When the disabled class was removed from the outer matCheckbox, the disabled class remained on the input, so even though the checkbox appeared not to be disabled, it couldn't accept any more input.

Issue Number: 9763

What is the new behavior?

Clicking on a disabled checkbox should not change it in any way.

Does this PR introduce a breaking change?

[ ] Yes
[x ] No

Other information

googlebot · 2018-02-13T22:22:10Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

smdunn · 2018-02-13T22:24:20Z

I signed it!

alfaproject · 2018-02-14T00:14:30Z

packages/upgrade/src/common/downgrade_component_adapter.ts

@@ -139,6 +141,7 @@ export class DowngradeComponentAdapter {
        (<OnChanges>this.component).ngOnChanges(inputChanges !);
      }

+      this.childComponentChangeDetector.markForCheck();


You can use the existing change detector: this.changeDetector.markForCheck();
And I suppose you need a unit test for this change as well

The values of this.componentRef.injector.get(ChangeDetectorRef) and this.componentRef.changeDetectorRef are not the same, and calling markForCheck on the this.changeDetector does not fix my bug.

Yeah, oddly this.changeDetector is not the wrapped component's actual change detector. Calling markForCheck on it has no effect (at least as far as this issue is concerned).

rkirov · 2018-02-15T19:35:58Z

Looks good given my limited understanding of ngupgrade internals. Would probably need a test too (do you need help with that?).

@gkalpak can you take an expert look at this?

gkalpak

Looks reasonable, but we do need some tests 😁
(LMK if you need help with that.)

gkalpak · 2018-02-15T20:44:16Z

packages/upgrade/src/common/downgrade_component_adapter.ts

@@ -60,6 +61,7 @@ export class DowngradeComponentAdapter {

    this.componentRef =
        this.componentFactory.create(childInjector, projectableNodes, this.element[0]);
+    this.childComponentChangeDetector = this.componentRef.injector.get(ChangeDetectorRef);


Is this really the child's ChangeDetector? I would actually think it is the component's ChangeDetector (or rather the ChangeDetector of the View within which this component resides). But I could be wrong 😁

It is the same instance that is injected into the wrapped component. So whatever that is.

gkalpak · 2018-02-15T20:50:29Z

BTW, I suspect this is related to #14286 (in the sense that the underlying issue is the same/similar). In which case, this comment (ignore the associated commit) would explain what is going on.

I am not sure I understand the usecase, so having a failing testcase or a demo would help.
(If you want to put together a live demo, you can use one of my stackblitz templates: ngUpgrade or ngUpgradeLite)

kseamon · 2018-02-15T21:41:40Z

The specific issue we encountered was with the [disabled] attribute of MatCheckbox not updating properly. One possibly confounding factor is that it is a property setter that sets the value of _disabled which is what the component actually uses. But that said, when we tried to reproduce the issue in a pure Angular (5) setting, it worked fine.

gkalpak · 2018-02-16T08:58:21Z

Are you using ngUpgradeLite or plain ol' ngUpgrade?
Any chance you could share a minimal reproduction (e.g. as a repo or on stackblitz (see my previous comment))?

smdunn · 2018-02-22T21:44:18Z

I added a test case that fails when the change I added is removed.

kseamon · 2018-02-23T14:43:22Z

Take a look at the unit test for a repro. I think this issues applies to both versions of ngupgrade, but we were using full when we encountered it.

gkalpak

This also fixes #14286 afaict.

gkalpak · 2018-03-02T09:49:59Z

Other than the CLA issue (and g3 sync), this is ready to go.

ngbot · 2018-03-20T20:58:06Z

Hi @smdunn! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-03-20T20:58:21Z

Hi @smdunn! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-03-20T20:58:48Z

Hi @smdunn! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

gkalpak · 2018-03-21T18:50:52Z

Seems like something went wrong. There are 141 commits on this PR now 😕

Fixes angular#14286

gkalpak · 2018-03-21T20:20:42Z

I fixed it up (and also renamed childComponentChangeDetector to viewChangeDetector, which is more accurate if I understand this comment correctly).

As soon as CI is green, I'll mark the PR for merge.
Thx again for working on this, @smdunn ✨

smdunn · 2018-03-21T20:22:08Z

Thanks for all the help with my first angular pull request @gkalpak

ngbot · 2018-03-22T13:19:43Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "cla/google" is failing
     status "google3" is pending
     1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Fixes #14286 PR Close #22209

alxhub · 2018-04-02T21:13:15Z

Author is a Googler, not sure why the CLA check is complaining.

angular-automatic-lock-bot · 2019-09-13T11:52:19Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: no label Feb 13, 2018

smdunn force-pushed the smdunn-matcheckbox-issue branch from f8df96a to cdc9296 Compare February 13, 2018 22:41

alfaproject suggested changes Feb 14, 2018

View reviewed changes

mhevery added the area: core Issues related to the framework runtime label Feb 14, 2018

rkirov requested a review from gkalpak February 15, 2018 19:35

gkalpak requested changes Feb 15, 2018

View reviewed changes

smdunn force-pushed the smdunn-matcheckbox-issue branch from 2466e86 to 8f2b675 Compare February 28, 2018 16:10

gkalpak changed the title ~~fix(matcheckbox): need to call markForCheck on child changeDetectorRef~~ fix(upgrade): need to call markForCheck on child changeDetectorRef Mar 1, 2018

gkalpak added type: bug/fix comp: upgrade/static target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 1, 2018

gkalpak approved these changes Mar 1, 2018

View reviewed changes

gkalpak force-pushed the smdunn-matcheckbox-issue branch from 8f2b675 to 744be99 Compare March 1, 2018 20:32

tinayuangao mentioned this pull request Mar 9, 2018

MatCheckbox disabled bug- changeDetectorRef markForCheck angular/components#9763

Closed

rkirov added cla: yes and removed cla: no labels Mar 21, 2018

fix(upgrade): correctly handle downgraded OnPush components

72a319c

Fixes angular#14286

gkalpak force-pushed the smdunn-matcheckbox-issue branch from 9d8ceeb to 72a319c Compare March 21, 2018 20:17

googlebot added cla: no and removed cla: yes labels Mar 21, 2018

gkalpak added cla: yes and removed cla: no labels Mar 21, 2018

googlebot added cla: no and removed cla: yes labels Mar 21, 2018

gkalpak added cla: yes and removed cla: no labels Mar 21, 2018

ocombe added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Mar 22, 2018

alxhub pushed a commit that referenced this pull request Apr 2, 2018

fix(upgrade): correctly handle downgraded OnPush components (#22209)

f43fba6

Fixes #14286 PR Close #22209

alxhub closed this in ad9ce5c Apr 2, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(upgrade): need to call markForCheck on child changeDetectorRef #22209

fix(upgrade): need to call markForCheck on child changeDetectorRef #22209

smdunn commented Feb 13, 2018 •

edited by mhevery

googlebot commented Feb 13, 2018

smdunn commented Feb 13, 2018

alfaproject Feb 14, 2018

smdunn Feb 14, 2018

kseamon Feb 14, 2018

rkirov commented Feb 15, 2018

gkalpak left a comment

gkalpak Feb 15, 2018

kseamon Feb 15, 2018

gkalpak commented Feb 15, 2018

kseamon commented Feb 15, 2018

gkalpak commented Feb 16, 2018

smdunn commented Feb 22, 2018

kseamon commented Feb 23, 2018

gkalpak left a comment

gkalpak commented Mar 2, 2018 •

edited

ngbot bot commented Mar 20, 2018

ngbot bot commented Mar 20, 2018

ngbot bot commented Mar 20, 2018

gkalpak commented Mar 21, 2018

gkalpak commented Mar 21, 2018

smdunn commented Mar 21, 2018

ngbot bot commented Mar 22, 2018

alxhub commented Apr 2, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

fix(upgrade): need to call markForCheck on child changeDetectorRef #22209

fix(upgrade): need to call markForCheck on child changeDetectorRef #22209

Conversation

smdunn commented Feb 13, 2018 • edited by mhevery

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Feb 13, 2018

smdunn commented Feb 13, 2018

alfaproject Feb 14, 2018

Choose a reason for hiding this comment

smdunn Feb 14, 2018

Choose a reason for hiding this comment

kseamon Feb 14, 2018

Choose a reason for hiding this comment

rkirov commented Feb 15, 2018

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Feb 15, 2018

Choose a reason for hiding this comment

kseamon Feb 15, 2018

Choose a reason for hiding this comment

gkalpak commented Feb 15, 2018

kseamon commented Feb 15, 2018

gkalpak commented Feb 16, 2018

smdunn commented Feb 22, 2018

kseamon commented Feb 23, 2018

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak commented Mar 2, 2018 • edited

ngbot bot commented Mar 20, 2018

ngbot bot commented Mar 20, 2018

ngbot bot commented Mar 20, 2018

gkalpak commented Mar 21, 2018

gkalpak commented Mar 21, 2018

smdunn commented Mar 21, 2018

ngbot bot commented Mar 22, 2018

alxhub commented Apr 2, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

smdunn commented Feb 13, 2018 •

edited by mhevery

gkalpak commented Mar 2, 2018 •

edited